Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various improvements to the exported files controller #292

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vuntz
Copy link
Member

@vuntz vuntz commented Feb 9, 2016

No description provided.

@@ -62,13 +62,11 @@

#support paths
get "utils(.:format)", controller: "support", action: "index", as: "utils"
get "utils/files/:id(.:format)", controller: "support", action: "destroy", constraints: { id: /[^\/]+/ }, as: "utils_files"
delete "utils/files/:id(.:format)", controller: "support", action: "destroy", constraints: { id: /[^\/]+/ }, as: "utils_files"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. 128/100

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this require some change in the cli?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not part of the CLI yet.

It's really horrible to put the files in the db directory, so don't do
that.

Also properly clean up files at the end.
While not strictly needed, it's slightly better in order to avoid
non-crowbar owned files in the rails app tmp directory. We use a chown
at the end, of course, but if the rails app is stopped in the middle,
then the tmp directory contains files that can't be removed.

Using a proper temporary directory helps (since it may be cleaned up on
reboot, or by scripts).
They were probably used a long time ago...
@rsalevsky
Copy link
Member

+1

exports = Dir.glob(Rails.root.join("db", "*.json").to_s)
cmd = ["tar", "-czf", Rails.root.join("tmp", filename).to_s, *exports]
tmpdir = Dir.mktmpdir
tmpdirpath = Pathname.new(tmpdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR: i would call it tmpdir_path for consistency

or as an alternative just use tmpdir = Dir.mktmpdir(Pathname.new(tmpdir)) and reference it with .to_s where needed

@MaximilianMeister
Copy link
Member

just unsure about the cli part (don't know if we even offer it)
other than that it looks good

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

8 participants